Conversation
|
@chrisdimoff @blb451 @jamesonji can you review this ticket? |
|
@kamork Hi Kitty. My pull request went in before yours. I had to change a few things to get multiple pictures to upload which will break your current carousel. To fix your carousel you should copy the code for the carousel from the integration branch and insert it into your newly designed show page. The crux of the issue is that we can no longer call game.picture . |
|
@kamork to add on to what @chrisdimoff said, you should prioritize the the carousel code from integration when resolving the merge conflict. However, keep your code for the rest of the page. Then, test if it all works. |
|
Added the new image carousel code, also made a few tweaks to divs and buttons. |
whilestevego
left a comment
whilestevego
left a comment
There was a problem hiding this comment.
Your PR looks good. However, the formatting of the HTML needs some sculpting. Chisel those corners and sand the edges! 🎨 🖌
| </p> | ||
| <div class="container-fluid" style="background-color: steelblue; padding-top: 5px"> | ||
| <div class="container-fluid" style="background-color: steelblue"> | ||
| <%# if can?(:manage, @game) %> |
There was a problem hiding this comment.
Can you delete this line?
| </div> | ||
| <% end %> | ||
|
|
||
| <%# if (can? :manage, @game ) %> |
There was a problem hiding this comment.
Delete this one too. It's not used 😎
| @@ -1,97 +1,105 @@ | |||
| <div class ="game-show"> | |||
There was a problem hiding this comment.
Can you remove the space between class =? Bridge the divide! ✊
| <div class="container-fluid"> | ||
| <div class="row"> | ||
| <div class="col-md-6 col-sm-6"> | ||
| <h2 style="font-weight:bold; margin-bottom: 20px; color: #EF7030;"><%= @game.title %></h2> |
There was a problem hiding this comment.
Avoid inlining styles. Can put this in a class? You could use a selector such as .game-show h2 if it makes sense for the page seeing how this h2 is inside div.game-show.
| <% end %> | ||
| </p> | ||
| <div class="container-fluid" style="background-color: steelblue"> | ||
| <%# if can?(:manage, @game) %> |
There was a problem hiding this comment.
Can you delete this line?
| </div> | ||
| </div> | ||
| <% end %> | ||
| </div> |
There was a problem hiding this comment.
The indent level on this closing div isn't quite right.
| <% end %> | ||
| </div> | ||
| <!-- end of game management --> | ||
| <div class="col-md-6 col-sm-6"> |
There was a problem hiding this comment.
Is this div used at all? Please fix the indenting. 🤓
| <ol class="carousel-indicators"> | ||
|
|
||
| <% @game.pictures.each_with_index do |pic, i| %> | ||
| <li data-target="#home-slideshow" data-slide-to= <%= "#{i}" %> |
There was a problem hiding this comment.
Remove the space after data-slide-to=
| <% @game.pictures.each_with_index do |picture, index| %> | ||
| <% url = picture.carousel.url %> | ||
| <% if index == 0 %> | ||
| <div class="item active"> |
There was a problem hiding this comment.
Indenting should be two spaces
|
|
||
| <!-- end of game stats --> | ||
| <% if user_is_gamer? %> | ||
| <div> |
There was a problem hiding this comment.
Too much indenting here.
2 participants
@whilestevego please take a look